Update profilePage to include timezone #1717

NikkiWines · 2021-03-11T01:20:19Z

Details

PR does a couple of things:

Removes the now unnecessary fetchTimezone() function and updates the personalDetailsonyx key so that each user's personalDetails also contains a timezone.
Updates the priorityMode onyx key to comply with how we want to save NVP onyx keys.
Updates the selectable area for opening the details page to be the avatar and the user's name (as requested here
Updates the Details page so that it now includes an abbreviated timezone name for clarity (ex. 10:51 AM PST instead of just 10:51 AM)

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/156248

Tests

For testing you're going to need two accounts:
Account A: This an account who on a policy with multiple different users
Account B: Someone who is on the same policy as account A.

Log into E.com with Account A
Navigate to Settings > Account > Preferences > Timezone and modify your timezone
Log into E.cash with Account A and check your myPersonalDetails onyx key to confirm it contains the correct timezone.
In E.cash, send a message to Account B
Confirm the message has the correct timestamp for your timezone.
Click on the name of Account B in the chat header (not on the avatar) and confirm it opens up the Details page.
Confirm you see Account B's timezone in the details.
In E.cash, navigate to Settings > Preferences and confirm the page loads as expected
Confirm you can modify your priority mode selection and that the nvp_priorityMode onyx key updates correctly.

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Video of Clicking on Chat Header

Screen.Recording.2021-03-19.at.10.20.12.AM.mov

…e from personalDetailsList not separate API call

…ki-personaldetails-timezone

update priorityMode nvp to have correct const

marcaaron

Changes look good! I just have a few questions about the set/merge changes.

src/ONYXKEYS.js

marcaaron · 2021-03-19T19:07:28Z

src/libs/actions/PersonalDetails.js

@@ -125,7 +112,7 @@ function fetch() {
    })
        .then((data) => {
            const allPersonalDetails = formatPersonalDetails(data.personalDetailsList);
-            Onyx.merge(ONYXKEYS.PERSONAL_DETAILS, allPersonalDetails);
+            Onyx.set(ONYXKEYS.PERSONAL_DETAILS, allPersonalDetails);


Why the change from merge() to set()?

I think there may be some personal details created that are not in the personalDetailsList e.g. those added when loading reports or creating a chat with a brand new user. Is there a chance we are overwriting those here?

Before we needed to merge otherwise I believe we'd overwrite the timeZone that was added to the personalDetails when we called fetchTimezone() but now the details returned from personalDetailsList and PersonalDetails_GetForEmails both contain the timeZone so there's no need to merge instead of set.

Is there a chance we are overwriting those here?

I suppose if we're getting different persoanlDetaials from the report, then yes we would be overwriting it, so it may be best to stick with merge over set in the end.

Ok cool. I think that's probably a safe bet here. It sort of makes sense to favor using merge() over set() unless there is a specific reason why we might want to blow away everything else we've got.

marcaaron · 2021-03-19T19:11:48Z

src/libs/actions/PersonalDetails.js

@@ -135,7 +122,7 @@ function fetch() {
            myPersonalDetails.lastName = lodashGet(data.personalDetailsList, [currentUserEmail, 'lastName'], '');

            // Set my personal details so they can be easily accessed and subscribed to on their own key
-            Onyx.merge(ONYXKEYS.MY_PERSONAL_DETAILS, myPersonalDetails);
+            Onyx.set(ONYXKEYS.MY_PERSONAL_DETAILS, myPersonalDetails);


Similar question about why set and not merge. But I can't think of any place we are modifying the myPersonalDetails I think probably after the timezone changes in this PR we are just always overwriting the details with those from the server on page refresh. Which seems fine?

marcaaron · 2021-03-19T19:53:43Z

Can't seem to get the timezone stuff to work (even after updating Auth). I don't have a great idea about why that would be but here's what I tried:

Clean make on Auth then make again
Change timezone for someone in e.com
Refresh page in e.cash and look at that guy's profile page
Timezone is still wrong

logged out response of personalDetailsList and a lot of stuff is just empty

Any ideas?

NikkiWines · 2021-03-19T20:33:11Z

Hmm, I'm not able to reproduce that issue.

Pre changes:

Post changes:

marcaaron · 2021-03-19T20:56:16Z

Ok after some back and forth @NikkiWines figured out that I must be testing with an account that did not have a personalDetails nvp set. We don't create one until the user modifies their avatar or name.

Maybe we can create a follow up issue to do something about that since it's a little confusing that we aren't picking up the tz in E.cash unless the user has modified some unrelated field first.

marcaaron

Tests good!

add priorityMode migration

NikkiWines · 2021-03-19T21:10:54Z

updated!

marcaaron

Looks great! We have to add the migration to this list here and then we are all set

https://github.com/Expensify/Expensify.cash/blob/546e0f24e05ef5b48ef0df988fa3ef5ccdc54219/src/libs/migrateOnyx.js#L10

NikkiWines · 2021-03-19T22:05:33Z

Oops, my bad forgot that part.

NikkiWines · 2021-03-19T22:06:39Z

updated!

…ki-personaldetails-timezone

NikkiWines · 2021-03-19T23:07:03Z

updated once more with conflicts resolved.

marcaaron

Looks great !

marcaaron · 2021-03-19T23:11:42Z

src/libs/migrations/RenamePriorityModeKey.js

+
+                Onyx.multiSet({
+                    priorityMode: null,
+                    [ONYXKEYS.NVP_PRIORITY_MODE]: oldPriorityMode,


Kinda of unrelated but random thought here is that we should probably not reference ONYXKEYS in migration files since the migration is "forever" even tho the constant in Onyx could possibly change. Nothing to do here just a weird thought I guess.

update myPersonalDetails and personalDetails onyx keys to get timezon…

f1d29bc

…e from personalDetailsList not separate API call

NikkiWines self-assigned this Mar 11, 2021

NikkiWines added 8 commits March 10, 2021 18:01

style

4d81cff

Merge branch 'master' of github.com:Expensify/Expensify.cash into nik…

38a5364

…ki-personaldetails-timezone

merge master, update AuthScreens.js to not call fetchTimeZone()

d027da0

undo change to User.js

d1daae6

add semicolons

f990e66

Merge branch 'master' of github.com:Expensify/Expensify.cash into nik…

e7e478e

…ki-personaldetails-timezone

update profilePage to detailsPage

9e34aad

update priorityMode nvp to have correct const

make both avatar and name selectable

4ce6b36

NikkiWines changed the title ~~Add timezone to personalDetails key~~ Rename DetailsPage and update it to include timezone Mar 19, 2021

NikkiWines added 2 commits March 19, 2021 10:22

undo file rename

dd91b65

update priorityMode onyx key

dd76c01

NikkiWines changed the title ~~Rename DetailsPage and update it to include timezone~~ Update profilePage to include timezone Mar 19, 2021

NikkiWines requested a review from a team March 19, 2021 17:33

NikkiWines marked this pull request as ready for review March 19, 2021 17:33

botify requested review from jasperhuangg and removed request for a team March 19, 2021 17:33

marcaaron reviewed Mar 19, 2021

View reviewed changes

use merge instead of set

c5fe238

add priorityMode migration

NikkiWines requested a review from a team as a code owner March 19, 2021 21:09

botify requested review from flodnv and removed request for a team March 19, 2021 21:09

marcaaron requested changes Mar 19, 2021

View reviewed changes

NikkiWines removed the request for review from flodnv March 19, 2021 22:05

include migration in all requisite areas

13a35e0

NikkiWines added 2 commits March 19, 2021 16:05

Merge branch 'master' of github.com:Expensify/Expensify.cash into nik…

3c22053

…ki-personaldetails-timezone

remove extra space

ded3189

marcaaron approved these changes Mar 19, 2021

View reviewed changes

NikkiWines merged commit 56b4a96 into master Mar 19, 2021

NikkiWines deleted the nikki-personaldetails-timezone branch March 19, 2021 23:35

github-actions bot locked and limited conversation to collaborators Mar 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update profilePage to include timezone #1717

Update profilePage to include timezone #1717

NikkiWines commented Mar 11, 2021 •

edited by gleitebr

Loading

marcaaron left a comment

marcaaron Mar 19, 2021

marcaaron Mar 19, 2021

NikkiWines Mar 19, 2021

marcaaron Mar 19, 2021

marcaaron Mar 19, 2021

marcaaron commented Mar 19, 2021

NikkiWines commented Mar 19, 2021

marcaaron commented Mar 19, 2021

marcaaron left a comment

NikkiWines commented Mar 19, 2021

marcaaron left a comment

NikkiWines commented Mar 19, 2021

NikkiWines commented Mar 19, 2021

NikkiWines commented Mar 19, 2021

marcaaron left a comment

marcaaron Mar 19, 2021

Update profilePage to include timezone #1717

Update profilePage to include timezone #1717

Conversation

NikkiWines commented Mar 11, 2021 • edited by gleitebr Loading

Details

Fixed Issues

Tests

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Video of Clicking on Chat Header

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron Mar 19, 2021

Choose a reason for hiding this comment

marcaaron Mar 19, 2021

Choose a reason for hiding this comment

NikkiWines Mar 19, 2021

Choose a reason for hiding this comment

marcaaron Mar 19, 2021

Choose a reason for hiding this comment

marcaaron Mar 19, 2021

Choose a reason for hiding this comment

marcaaron commented Mar 19, 2021

NikkiWines commented Mar 19, 2021

marcaaron commented Mar 19, 2021

marcaaron left a comment

Choose a reason for hiding this comment

NikkiWines commented Mar 19, 2021

marcaaron left a comment

Choose a reason for hiding this comment

NikkiWines commented Mar 19, 2021

NikkiWines commented Mar 19, 2021

NikkiWines commented Mar 19, 2021

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron Mar 19, 2021

Choose a reason for hiding this comment

NikkiWines commented Mar 11, 2021 •

edited by gleitebr

Loading